Skip to content

[MNT] Diagnose and address long test runtimes (#1633)#1692

Open
Abhishek9639 wants to merge 3 commits intoopenml:mainfrom
Abhishek9639:mnt/diagnose-long-test-runtimes
Open

[MNT] Diagnose and address long test runtimes (#1633)#1692
Abhishek9639 wants to merge 3 commits intoopenml:mainfrom
Abhishek9639:mnt/diagnose-long-test-runtimes

Conversation

@Abhishek9639
Copy link

@Abhishek9639 Abhishek9639 commented Feb 26, 2026

Fixes #1633

Changes

Current CI test runs take 1–2+ hours. This PR diagnoses the bottleneck and implements several improvements:

Root Cause

The production_server tests (74 tests) make live API calls to openml.org, taking ~1h 23m in CI even with 4-worker parallelization.

Improvements

  1. Global per-test timeout (pyproject.toml)

    • Added timeout = 600 (10 min) to [tool.pytest.ini_options]
    • Prevents any single test from hanging indefinitely

  2. CI workflow improvements (.github/workflows/test.yml)

    • Changed --durations=20 → --durations=0 to report ALL test durations for diagnosis
    • Added explicit --timeout=600 to all 3 pytest invocations

  3. Fixture optimization (tests/conftest.py)

    • Changed verify_cache_state fixture scope from function → module
    • Reduces redundant filesystem I/O (was running before/after EVERY test)

  4. Benchmark script (scripts/profile_tests.sh)

    • New script for easy local test duration profiling
    • Configurable marker filters

Test Distribution Analysis

Category Count CI Time
All tests 368
production_server 74 ~1h 23m (bottleneck)
test_server 196 excluded from CI
sklearn-only 6 ~1 min
Non-server 99 fast

Verification

  • All pre-commit checks pass (ruff, ruff-format, mypy)
  • All 368 tests still collect correctly

[MNT] Diagnose and address long test runtimes (openml#1633)
07243d1 
- Add global per-test timeout (600s) to pytest config
- CI: report all test durations (--durations=0) for diagnosis
- CI: add explicit --timeout=600 to prevent hanging tests
- Optimize verify_cache_state fixture: scope function -> module
- Add scripts/profile_tests.sh for local duration profiling
@Abhishek9639
Copy link
Author

Abhishek9639 commented Feb 26, 2026 •
edited
Loading

Hii @geetu040 and @fkiraly,
Fixed the code quality checks. All pre-commit checks are now passing.
Please review it.

geetu040
geetu040 suggested changes Mar 1, 2026
View reviewed changes
Copy link
Collaborator

@geetu040 geetu040 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, scripts/profile_tests.sh file will be used, but I not sure about other duration and timeout related changes. See comments below.

.github/workflows/test.yml Outdated
fi

pytest -n 4 --durations=20 --dist load -sv $codecov -o log_cli=true -m "$marks"
pytest -n 4 --durations=0 --timeout=600 --dist load -sv $codecov -o log_cli=true -m "$marks"
Copy link
Collaborator

@geetu040 geetu040 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

durations: should not be set to 0, we could possibly use this in CI
timeout: we need to think about this, I would try to avoid setting this explicitly, but let's discuss. what's your reasoning to set a timeout here?

Copy link
Author

@Abhishek9639 Abhishek9639 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, I've reverted both changes durations is back to 20 and removed timeout from CI.

scripts/profile_tests.sh
Copy link
Collaborator

@geetu040 geetu040 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this file looks fine and we probably need this
duration, timeout and output file path should take value from users like markers and should have default value

Copy link
Author

@Abhishek9639 Abhishek9639 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update done The script now supports m (marker), d (durations), t (timeout), and o (output file) as CLI arguments, each with sensible default values. Let me know if you'd like any tweaks.

tests/conftest.py Outdated


@pytest.fixture(autouse=True, scope="function")
@pytest.fixture(autouse=True, scope="module")
Copy link
Collaborator

@geetu040 geetu040 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why did you have to change this?

Copy link
Author

@Abhishek9639 Abhishek9639 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right, this change wasn't necessary. I've reverted it back to scope="function". Thanks for pointing it out

Address review feedback: revert CI/conftest changes, improve profile …
ede2e0d 
…script

- Revert CI workflow to original --durations=20 (no timeout)
- Remove global timeout from pyproject.toml
- Revert conftest.py verify_cache_state scope to function
- Update profile_tests.sh: accept CLI args (-m, -d, -t, -o) with defaults
@geetu040
Copy link
Collaborator

geetu040 commented Mar 1, 2026

you should mention the issue #1633 without the keyword Fixes #1633 since it doesn't close it, rather adds script to help debug this.

@Abhishek9639
Copy link
Author

Abhishek9639 commented Mar 1, 2026

@geetu040,
I’ve made all the changes you suggested. Could you please review it once?
And if any further changes are needed, please let me know.
Thanks

geetu040
geetu040 suggested changes Mar 1, 2026
View reviewed changes
Copy link
Collaborator

@geetu040 geetu040 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

see the comment below

scripts/profile_tests.sh
Comment on lines 43 to 48
pytest \
--durations="$DURATIONS" \
--timeout="$TIMEOUT" \
-q \
-m "$MARKER_FILTER" \
2>&1 | tee "$OUTPUT_FILE"
Copy link
Collaborator

@geetu040 geetu040 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would also prefer additional argument -n for more workers and remove -q to get full pytest output

pytest \
+ --dist=load \
+ -n=$NUM_WORKERS \
  --durations="$DURATIONS" \
  --timeout="$TIMEOUT" \
- -q \
  -m "$MARKER_FILTER" \
  2>&1 | tee "$OUTPUT_FILE"

This would mimic the exact pytest command in CI

Copy link
Author

@Abhishek9639 Abhishek9639 Mar 1, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for pointing out the changes.
I’ll make the changes.

Update profile_tests.sh: add -n workers, --dist=load, remove -q
8a00373 
- Add -n flag for parallel workers (default: 4)
- Add --dist=load to distribute tests across workers
- Remove -q flag for full pytest output
- Mimics exact pytest command used in CI
@Abhishek9639
Copy link
Author

Abhishek9639 commented Mar 1, 2026

@geetu040,
Updated Added -n for workers with --dist=load and removed -q for full output the script now mimics the exact CI pytest command. Please review
If any further changes are needed, please let me know.
Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@geetu040 geetu040 geetu040 requested changes

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[MNT] diagnose and address long test runtimes

2 participants

@Abhishek9639 @geetu040